Conversation
…able results (apache#3896) Generated-by: Command Code
…abstraction-layer-for-cross-platform-backends-
…vice The option was declared on MakaCuServiceOptions but no caller could reach it. MakaCuBackendOptions never declared the field, and createMakaCuBackend never forwarded it, so neither the selector nor the Desktop composition could set it. That left an interface promise with a host default, no consumer, and no test able to tell a correct value from an empty argv or a doubled host argument. host is still required and still hardcoded at the spawn site, with its rationale unchanged. The seam comes back when a second platform actually needs a different argv, driven by a real requirement. Generated-by: Command Code
…ross-platform-backends-
hqhq1025
left a comment
There was a problem hiding this comment.
Technical conclusion: NO-GO due to one P2.
This change adds a darwin -> maka-cu platform binding, platform injection seams for cross-platform tests, typed unavailable results, and removes the unused child-argument seam. The fail-closed selector behavior works, but the new unavailable reason is dropped before Desktop capability reporting, so the user-facing distinction described by this PR is not implemented.
Validation completed on this exact head: build:test; full typecheck, lint, format, ASF-header, locale, TUI-copy, Astryx, renderer-architecture (112/112), and E2E-budget checks; Computer Use 120/120; Desktop 2492/2492; focused changed-path tests 67/67; Electron E2E 38/38; and git diff --check. The head cleanly merges with current main c22768c3b0dc47518f6f8584e864f86f0b1e5379. GitHub currently exposes only the successful label check, not a hosted test check.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
…ting Selection grew a typed `unavailableReason` so a capability surface could say why Computer Use is off, but no production path read it: the Desktop boot forwarded only `backendId` to `computerUseServiceHealth()`, which mapped every `none` to `cu_executor_undistributable`, and the capability's feature layer reported `cu_artifact_missing` for the same case. An unbound platform therefore reached the Permission/Health UI as a failed integrity check — the three states the reason distinguishes arrived as one copy. The reason now survives the whole path. `computerUseServiceHealth()` takes it as a third argument and projects `unsupported_platform` to the new `cu_platform_unsupported` code, `backend_failed` to `cu_backend_unavailable`, and `missing_executable` to `cu_executor_undistributable`; a `none` carrying no typed reason keeps the undistributable code it had. Both capability layers name that same cause, so the feature row no longer contradicts the probe row. The row projection lives in `computer-use-capability-reasons.ts` rather than in `capability-snapshot.ts` because the snapshot module imports Electron, which a `node --test` process cannot load; keeping the projection out of that import graph is what makes it assertable, the same reason `app-icon-ipc` injects Electron instead of importing it. macOS behaviour is unchanged: `darwin` still selects `maka-cu` with the same tools and the same binary/digest checks. Generated-by: Command Code Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
…selection The typed reason is only worth carrying if the projection keeps the three ways to have no backend apart, so these tests assert what the capability surface shows. The first pins the reason-code mapping, including that a `none` with no typed reason keeps the undistributable code, and that a platform with no binding reaches the health projection as a platform fact. The second composes that projection with the row and asserts the three reasons arrive as three different codes, with the live executor and the never-assembled snapshot keeping their own artifact and probe reasons. Generated-by: Command Code Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
|
@Astro-Han Please take a look when you have time,thank you! |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — the shape of this reads well, and in particular the reason now survives the whole path (selectComputerUseBackend → native.computerUse → computerUseServiceHealth → computerUseCapabilityReasons → locale copy), which is the link that usually gets dropped. Two comments and a few nits.
The ?? process.platform default is now the only untested branch
Every selectComputerUseBackend / createComputerUseHost call in the suites now passes platform explicitly (packages/computer-use/src/__tests__/select-backend-host-events.test.ts:48,93,109,128,133,146, packages/computer-use/src/__tests__/maka-cu-backend.test.ts:1556,1568,1585, apps/desktop/src/main/__tests__/computer-use-host.test.ts:99,109,122,138,150,178), so nothing pins the seam default at packages/computer-use/src/select-backend.ts:157 or apps/desktop/src/main/computer-use-host.ts:75. A regression there — deps?.platform ?? 'darwin', or dropping the ?? — would keep every suite green on every CI OS while turning the platform decision back into a constant, which is exactly the failure this PR removes. Previously the Darwin assertions exercised process.platform implicitly; after this change that path has no test at all.
One assertion per entry point would close it, e.g.:
// no `platform` seam: the host's own platform decides
assert.equal(
selectComputerUseBackend({ binaryPath: '/tmp/fake', expectedBinarySha256: '0'.repeat(64), createBackend: () => stub() }).backendId,
process.platform === 'darwin' ? 'maka-cu' : 'none',
);unavailableReason isn't tied to backendId: 'none' by the type
select-backend.ts:85-96 permits any combination, while its own doc says "A missing reason means a backend is live". computerUseServiceHealth reads the reason only on the 'none' branch (computer-use-host.ts:174), so a caller passing a reason alongside a live backendId is silently ignored rather than rejected at compile time. Since the point of the PR is that this is a typed fact, a union states it once (shape flexible, emptyTools() stays shared by both arms):
type SelectedComputerUseBackend =
| { backendId: 'none'; unavailableReason: CuUnavailableReason; tools: ComputerUseToolSet }
| { backendId: CuBackendId; backend: DisposableBackend; tools: ComputerUseToolSet };Only select-backend.ts constructs this value today, so the change is contained. It would also let runtime-host-boot.ts:1749-1756 pass the selection through instead of threading a third positional argument, which is what currently allows the pairing to drift between the two call sites.
Nits
- Two places now name the shipped executor.
select-backend.ts:186returnsbinding.id, butDEFAULT_CU_BACKEND_IDis still exported and still asserted (maka-cu-backend.test.ts:1583), and the comment above the host's selection call still reads "the host takes whateverDEFAULT_CU_BACKEND_IDnames". They agree because there is one row; if a second row ever ships a different id, the constant and that comment go stale with nothing failing. Deriving the constant from the table — or rewording the comment to nameCU_PLATFORM_BACKEND_BINDINGSas authoritative — keeps the "one decision" property the comment is protecting. platformLabelhas no reader. It is documented as "Human label for capability reporting", but the capability layer only ever emitscu_platform_unsupported(computer-use-capability-reasons.ts:43-45,capability-snapshot.ts:136-160).CU_PLATFORM_BACKEND_BINDINGS/CuPlatformBackendBindingare also exported frompackages/computer-use/src/index.tswith no consumer outsideselect-backend.ts;knip.jsononly coversapps/desktopandpackages/ui, so no gate will flag it. Fine as a forward declaration — just worth adding when the copy actually wants it.- "Production callers omit them" (
select-backend.ts:146-150, README "Platform abstraction") is no longer literally true: the Desktop host always passesplatform(computer-use-host.ts:75). A clause like "…or a host may forward its own resolved platform" would keep the doc honest.
Verified, no action needed
- The three no-backend states really are distinct end-to-end:
unavailableReasonCode(computer-use-host.ts:151-156) maps them tocu_platform_unsupported/cu_backend_unavailable/cu_executor_undistributable, andcomputerUseCapabilityReasonsmakes the feature and probe rows agree for anoneselection instead of reportingcu_artifact_missing. - Copy completeness is enforced rather than hand-checked:
CapabilityReasonCopy = Record<CapabilityReasonCode, string>plusapps/desktop/src/main/__tests__/health-center-copy.test.ts:103-121, which requires non-fallback copy for every code in all three locales. All three got the new string (capability-reason-copy.ts:34,59,84). - Selection precedence is right: the binding lookup (
select-backend.ts:158) runs before the digest check, so an unbound platform reportsunsupported_platformrather thanmissing_executable, and the packaged/digest failures on Darwin still land on the undistributable code they had before. - The newly un-skipped tests are CI-OS-safe:
createMakaCuBackendandMakaCuServicehave noprocess.platformgate and spawn lazily, so an injectedplatform: 'darwin'only steers selection and never reaches a macOS-only API. Since verification reports a macOS run, the first Linux/Windows CI run is what proves the "runs on every CI OS" claim. - Commit 2 adds
childArgsand then removes it, so the spawn site is unchanged (['host']) in the net diff — only churn, no review needed.
Summary
Computer Use named its executor with a hardcoded
process.platform !== 'darwin'check, so three different outcomes — unsupported platform, missing executor,
failed backend — collapsed into the same untagged
none, and adding a platformcould only mean copying the supervisor. This makes the platform decision an
explicit binding table and makes every "no backend" result typed.
CU_PLATFORM_BACKEND_BINDINGS(today the singledarwin → maka-curow) is theone place a platform names its executor;
selectComputerUseBackendconsults itinstead of testing
process.platforminline.SelectedComputerUseBackendnow carriesunavailableReason(
unsupported_platform|missing_executable|backend_failed), so acapability surface can say why Computer Use is off instead of guessing.
MakaCuSelectionandcreateComputerUseHosttake aplatformseam thatdefaults to
process.platform. The Darwin selection assertions that used toskip themselves off-macOS (
if (process.platform !== 'darwin') return;) nowrun on every CI OS.
MakaCuServiceremains the only supervised-child lifecycle for everymaka.cu/2executor. The unusedchildArgsseam is dropped — nothing couldreach it (
MakaCuBackendOptionsnever declared the field andcreateMakaCuBackendnever forwarded it) — and the header comment now statesthat platform policy belongs to the backend and the Desktop composition, never
to the process lifecycle.
macOS behaviour is unchanged:
darwinstill selectsmaka-cuwith the sametools and the same binary/digest checks. Non-Darwin previously returned an
untagged
none; it now returnsnoneplusunavailableReason: 'unsupported_platform'.Refs #3896
Review focus
The issue proposes a
ComputerUseBackendinterface with per-OS implementationsand Linux/Windows stubs. This PR deliberately takes the other route: a backend
that silently no-ops is the failure mode selection exists to prevent, and all
three platforms share one
maka.cu/2contract and one supervisor, so their onlyreal divergence is which native binary the Desktop provisions. Adding a platform
is therefore a row in the binding table plus its native executor and Desktop
provenance work — not a second backend implementation. An unbound platform fails
closed with a typed reason instead. If the named interface is wanted anyway,
please say so on the issue and I will follow it.
Verification
Run on the merge commit
6ee1b186b(macOS 15 arm64, Node 22.23.2), from therepository root:
npm run lint— pass (3591 files);npm run format:check— pass (2122 files)npm run clean && npm run build— pass;npm run typecheck— pass for every workspacepackages/computer-use: 120 tests pass (node --test "dist/**/*.test.js")apps/desktop: 2636 tests pass(
node --test --test-force-exit "dist/main/**/*.test.js" scripts/dev-app-runtime.test.mjs scripts/vite-workspace-packages.test.mjs)node scripts/ci-test-plan.mjs --base origin/main --head HEADselectscode=true,e2e=trueand thepackages/computer-use,apps/desktopworkspaces for this diff; the other
code=truegates pass as well —check:tui-copy,check:renderer-architecture --base,check:locale-hygiene --base,astryx:theme --check,astryx:surface-inventory, andknipforapps/desktopandpackages/ui(
new-task-reload.spec.ts—sessions:archivereported a live derived effectafter 38 windows had already run). That spec passes in isolation (2/2) and
exercises no Computer Use path.
Not run: the Storybook lane (not selected by the planner), the packaged and
notarized Computer Use path against a real window, and
check:asf-headers—locally it flags only the untracked
.commandcode/workspace directory, which isnot part of this branch.
AI use
Select exactly one:
Tool(s) and scope: Command Code wrote the platform binding selection, the typed
unavailable reasons, the
platformseams and their tests; the author reviewed,edited and verified the result. Both commits carry
Generated-by: Command Code.Checklist
Does this PR entail a change in behavior?